Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added option to provide a custom config file to the lsp. #460

Merged
merged 7 commits into from
Jul 18, 2021

Conversation

luctius
Copy link
Contributor

@luctius luctius commented Jul 17, 2021

This allows for a runtime/lsp_configs/.json file to be provided. When detected, it will be send to the lsp, allowing to user to configure the lsp.

Feedback is welcome, especially regarding the place of the file read; the naming of the directory and if this is the correct approach.

I added a sample file to facilitate discoverability.

Comment on lines 321 to 328
Err(e) => {
log::info!(
"Loading lsp config file for {} failed => {}",
language_config.language_id,
e
);
None
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to show that it failed when the custom config isn't there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah agreed

Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know it is this easy. Looks good to me other than comment I left. Tested but not sure if it works, probably it works.

But should we make use of languages.toml to do the custom configuration instead? kak-lsp seemed to do it with set-option -add global lsp_server_configuration rust.clippy_preference="on" but I wonder what are the limitations.

Maybe @ul have a better idea. @ul sorry for pinging you even but how was the choice to have custom configuration rather than doing the original json like object in kak-lsp?

@@ -410,6 +426,17 @@ impl LspProgressMap {
}
}

fn load_lsp_config_file(language: &str) -> anyhow::Result<Option<serde_json::Value>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of extra files, what do you think about making this a config key under the language_server toml key? Then one can use

config = """
{
   ...
}
"""

to declare the config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my first thought, (my second was to put it in the helix config file), but I decided against that because editing json in a toml file wasn't very ergonomic. That said, I am not against it; I will make that chance.

@luctius
Copy link
Contributor Author

luctius commented Jul 17, 2021

Tested but not sure if it works, probably it works.

You can easily test it by starting helix with RA_LOG=rust_analyzer=info hx -v <file.rs>. Then the log-file should show rust-analyzer output stating that the user config was received.

@ul
Copy link

ul commented Jul 17, 2021

Maybe @ul have a better idea. @ul sorry for pinging you even but how was the choice to have custom configuration rather than doing the original json like object in kak-lsp?

The idea was to make it easier to dynamically define these values inside Kakoune config without involving an external JSON processor (as Kakoune doesn't have a built-in JSON handling API available in configs), at least in simple cases. How wise was it and how widely is it leveraged in the wild? I don't know, I personally never used it. Maybe having it as original JSON and relying on piping through jq for dynamic modifications is more viable.

@@ -35,6 +35,7 @@ pub struct LanguageConfiguration {
pub scope: String, // source.rust
pub file_types: Vec<String>, // filename ends_with? <Gemfile, rb, etc>
pub roots: Vec<String>, // these indicate project roots <.git, Cargo.toml>
pub custom_config: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: let's just call it config.

@luctius luctius force-pushed the features/lsp_custom_config branch from 1f9f273 to bbe6b43 Compare July 18, 2021 06:31
@archseer archseer merged commit 0aa4390 into helix-editor:master Jul 18, 2021
@luctius luctius deleted the features/lsp_custom_config branch July 18, 2021 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants